fix(relay): delivery and ACK accept 64-hex delivery ids; acceptance markers are per branch (#508) - #511

Review — tpsdev-ai/cli PR #511 (head 05f238a), Closes #508

Repo visibility checked via repos/tpsdev-ai/cli .visibility: public (re-verified before posting). Findings below are diff-readable; nothing withheld.

Verified in a worktree at head 05f238a: built agent-then-cli (the root build's ordering), then test/relay-delivery-loss.test.ts through the isolated launcher — 28/28 pass, covering both sync and connect entry points, both id shapes, the cross-branch case, the malformed-id refusal, and the legacy flat marker.

Focus areas

Marker path safety [packages/cli/src/utils/relay.ts:375-378]. branchId is gated by /^[a-zA-Z0-9_-]+$/ — byte-identical to the pattern lookupBranch uses (identity.ts:484) — so no /, no . (hence no ..), no NUL; the marker's second component body.id is schema-constrained to UUID or 64-hex, both filename-safe. The path cannot escape .relay-accepted/. The namespace uses the channel's branch id at both call sites (syncRemoteBranch/connectAndKeepAlive pass their own branchId; nothing from the message body is consulted), so a message arriving over branch A's authenticated channel is marked under A regardless of body.from — a sender cannot pre-mark or suppress another branch's namespace.

Upgrade read of flat markers [relay.ts:380]. The legacy flat marker is checked alongside the namespaced one and suppresses redelivery while still ACKing — pinned by "a UUID accepted under the earlier unscoped marker layout is acknowledged without a second inbox record". Note 64-hex ids can never have flat markers (the pre-upgrade schema refused them outright), so the flat read only ever matches UUIDs.

Cross-branch 64-hex collision [relay.ts:377]. Namespaced by branch and pinned for both shapes and both entry points ("the same … delivery id from two branches is delivered for each"). A 64-hex id from one branch cannot suppress another branch's message.

Id pattern [wire-mail.ts:13]. z.string().uuid() (zod's anchored regex) union ^[a-f0-9]{64}$ (anchored, exact, lowercase-only). The malformed-id test pins not-a-delivery-id, uppercase hex (AB×32), and 65-char hex all refused with no ACK, no inbox record, and none of the ids or payload in the logs. It matches outbox.ts's own producer-side gate (/^[a-f0-9]{64}$/, outbox.ts:27) — the shapes agree at both ends.

Refusal log [relay.ts:421-425]. logRefusedDelivery emits the branch id and the failing field names from issue.path only — no values; the test asserts neither the payload nor the malformed ids appear. The invalid branch id throw at relay.ts:375 includes body.id, but only after body.id has passed the id parse at :374, so it is always a well-formed UUID/64-hex — nothing unsafe can reach that log line.

The id shape changes nothing else. The union only widens the accepted id set; delivery, marker naming, and ACK treat the id opaquely, and the per-shape tests assert behavioral equivalence end-to-end: one inbox record, one ACK, drained outbox, replay suppressed (with the duplicate still ACKed so the sender's acknowledgeOutbox record.id match drains — the sender side also accepts 64-hex via MailAckBodySchema, closing the loop that motivated #508).

Signature change. deliverRelayedToLocal(body) → (branchId, body) is exposed through the package exports (@tpsdev-ai/cli/utils/relay), but I found no consumer outside relay.ts anywhere in this repo, tpsdev-ai/bob, tpsdev-ai/flair, or the agent package — the only call site (acceptRelayedMail) is updated in this PR. "No released consumer" holds for every org repo I can check; external downstream consumers are unknowable from here, so the change rides a major-ish surface — worth a changelog note if you care about semver hygiene, but nothing blocking.

Verdict

APPROVE. The marker namespace is derived solely from the authenticated channel's branch, validated by the same pattern as key lookup, and pinned against traversal by construction; the flat-marker upgrade read prevents the lost-ACK duplicate; the id schema is anchored, exact, consistent with the producer, and refusal logging discloses field names only. The new tests cover every focus property under both entry points.

Sherlock review — PR #511, head 05f238a4 (Closes #508). Verdict: APPROVE.

Repo checked: repos/tpsdev-ai/cli → .visibility = "public". Author tps-flint (a tps-* agent), so normal internal review, not the external-author read-only path. Worktree ~/work/review-511-sherlock @ 05f238a4, agent+cli built, test/relay-delivery-loss.test.ts run (28 pass, 0 fail) plus the mutations below.

A 64-hex id from one branch can never suppress or replace another branch's message. Confirmed.
Markers are namespaced by the authenticated branch — packages/cli/src/utils/relay.ts:377-380:

const acceptedDir = join(getMailDir(), ".relay-accepted", "by-branch", branchId);
const marker = join(acceptedDir, body.id);
if (existsSync(marker) || existsSync(join(getMailDir(), ".relay-accepted", body.id))) return false;

branchId is the transport's authenticated peer — the channel is opened per registered branch in syncRemoteBranch (relay.ts:626) and connectAndKeepAlive (:700) — not a field the sender supplies, so a remote cannot write into another branch's namespace. Mutation: revert to a flat marker dir → all four "the same <shape> delivery id from two branches is delivered for each" cases fail (uuid and 64-hex, both sync and connect), so the branch scoping is the guard and it is tested.
The legacy flat read cannot let a 64-hex id suppress another branch's message: before this PR MailDeliverBodySchema.id was z.string().uuid(), so no 64-hex delivery was ever accepted or recorded in the flat layout — every flat marker is a UUID, and a 64-hex id can never equal a UUID. Mutation: drop the || existsSync(join(..., body.id)) read → "a UUID accepted under the earlier unscoped marker layout…" fails, so the upgrade path is exercised.

The id pattern is anchored and exact. Confirmed.
packages/cli/src/utils/wire-mail.ts:13 — const DeliveryIdSchema = z.union([z.string().uuid(), z.string().regex(/^[a-f0-9]{64}$/)]); — used for MailDeliverBodySchema.id (:16) and MailAckBodySchema.id (:25). Both anchors present; lowercase hex only, exactly 64. Mutation: drop the trailing $ (/^[a-f0-9]{64}/) → "a malformed delivery id is refused and logged without the payload" fails, because "ab".repeat(32) + "a" (65 chars) is then accepted. The test's near-misses ("AB".repeat(32) uppercase, the 65-char string, "not-a-delivery-id") pin the exactness.

No payload content reaches the refusal log. Confirmed.
packages/cli/src/utils/relay.ts:422-425 — logRefusedDelivery(branchId, error: ZodError) logs only error.issues.map((issue) => issue.path.map(String).join(".") || "body") — field paths, never values — and the function does not receive the message body at all. It is called from both syncRemoteBranch (:649) and connectAndKeepAlive (:774). Mutation: add console.error("LEAK", JSON.stringify(msg.body)) at the sync call site → the sync malformed case fails (payload-text and the ids appear in the log), so the "without the payload" assertion bites. The older handleIncomingMail path (:349) keeps its value-free Invalid MAIL_DELIVER from ${branchId} message.

The id shape changes nothing else about how a delivery is handled. Confirmed.
The only functional changes are the shared DeliveryIdSchema and the marker path; sendMessage, the dead-letter classification, the ACK and the inbox write are untouched, and deliverRelayedToLocal runs the same sequence (schema parse → marker lookup → sendMessage → DLQ on failure → marker write) for both shapes. The "a <shape> delivery id produces one inbox record, one ACK and an empty outbox" and replay cases pass identically for uuid and 64-hex.

Signature change. deliverRelayedToLocal gained a leading branchId; its only consumers are relay.ts itself (acceptRelayedMail, :416) and the test, and relay.ts is not re-exported from packages/cli/src/index.ts — no released consumer.

Could not see / note. I ran test/relay-delivery-loss.test.ts only, not the full cli suite. The test launcher's HOME-isolation guard printed concurrent metadata changes under the operator ~/.tps during my run (connections/{dtrt-pulse,tps-anvil,tps-kern,tps-sherlock}.json, logs/mail-deliver-health.log, secrets, tunnel-watchdog/tps-anvil.fails). Those are live-agent heartbeats on this host, not writes from this test — the test uses a temp TPS_MAIL_DIR and the launcher's isolated root — but I am flagging it because the guard surfaced it, not because I attribute it to this PR.